Skip to content

ARROW-10490: [C++][GLib] Fix range-loop-analysis warnings#8587

Closed
kou wants to merge 1 commit into
apache:masterfrom
kou:xcode-12
Closed

ARROW-10490: [C++][GLib] Fix range-loop-analysis warnings#8587
kou wants to merge 1 commit into
apache:masterfrom
kou:xcode-12

Conversation

@kou

@kou kou commented Nov 4, 2020

Copy link
Copy Markdown
Member

They are detected by Xcode 12.

@kou
[C++][GLib] Fix range-loop-analysis warnings
df7465e 
They are detected by Xcode 12.
@kou kou mentioned this pull request Nov 4, 2020
Closed
@github-actions

github-actions Bot commented Nov 4, 2020

Copy link
Copy Markdown

https://issues.apache.org/jira/browse/ARROW-10490

terencehonles
terencehonles approved these changes Nov 4, 2020
View reviewed changes

@terencehonles terencehonles left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Definitely better than pinning Xcode, and now I was going to look into the JNI status check failures.

I've bisected it to #8533, and I'm testing reverting the DCHECK_NE change to see what that does. I just finished bisecting so I was going to run the tests and then ask about if the change should be there and how the test should probably be fixed if it should be there.

@kou

kou commented Nov 4, 2020

Copy link
Copy Markdown
Member Author

OK.
I'll merge this.

It seems that #8533 isn't related to the JNI failure because #8533 is R related change and the JNI failure is Java related code.
We may need to discuss it at the https://lists.apache.org/thread.html/rdd900f5359fb57f32e3eb686f6584535e81e3b2f7440cbc74f945b67%40%3Cdev.arrow.apache.org%3E thread.

@kou kou closed this in b4ddc96 Nov 4, 2020
@kou kou deleted the xcode-12 branch November 4, 2020 04:57
@terencehonles

terencehonles commented Nov 4, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

OK.
I'll merge this.

It seems that #8533 isn't related to the JNI failure because #8533 is R related change and the JNI failure is Java related code.
We may need to discuss it at the https://lists.apache.org/thread.html/rdd900f5359fb57f32e3eb686f6584535e81e3b2f7440cbc74f945b67%40%3Cdev.arrow.apache.org%3E thread.

It's an R related change, but it touches a C file which breaks the java/adapter/orc/src/test/java/org/apache/arrow/adapter/orc/OrcReaderTest.java test (it seems like it's a segfault, but I don't know how to get better logging to confirm that, the process is dead though). Removing the DCHECK_NE change in memory.cc "fixes" the issue, but it looks like there is also a memory leak coming from java/flight/flight-core/src/test/java/org/apache/arrow/flight/TestDoExchange.java (testClientCancel) which was also causing tests failures in a different test suite. I'll try to follow up on the mailing list tomorrow (thanks for the heads up about it, I just figured I'd share my findings here)

@kou

kou commented Nov 4, 2020

Copy link
Copy Markdown
Member Author

Ah, I see.

I'll try to follow up on the mailing list tomorrow

Thanks!

@asfimport asfimport mentioned this pull request Nov 4, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@terencehonles terencehonles terencehonles approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@kou @terencehonles